Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create barbican keystone listener deployment #34

Merged

Conversation

mauricioharley
Copy link
Contributor

No description provided.

@mauricioharley mauricioharley force-pushed the barbican_keystone_listener branch 2 times, most recently from 64f9074 to 707b197 Compare October 10, 2023 09:15
@mauricioharley mauricioharley force-pushed the barbican_keystone_listener branch 2 times, most recently from a2233ee to 22e3c2c Compare October 19, 2023 16:01
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll want to add Owns(&barbicanv1beta1.BarbicanKeystoneListener{}) to the SetupWithManager() func

}

// Add a finalizer to prevent user from manually removing child BarbicanAPI
controllerutil.AddFinalizer(deployment, helper.GetFinalizer())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're adding a finalizer to the BarbicanKeystoneListener resource, we will also need to remove the finalizer in our deletion logic. i.e. like we do here for the BarbicanAPI resource that this Barbican resource owns:

// Remove finalizers from any existing child GlanceAPIs
barbicanAPI := &barbicanv1beta1.BarbicanAPI{}
err = r.Get(ctx, types.NamespacedName{Name: fmt.Sprintf("%s-api", instance.Name), Namespace: instance.Namespace}, barbicanAPI)
if err != nil && !k8s_errors.IsNotFound(err) {
return ctrl.Result{}, err
}
if err == nil {
if controllerutil.RemoveFinalizer(barbicanAPI, helper.GetFinalizer()) {
err = r.Update(ctx, barbicanAPI)
if err != nil && !k8s_errors.IsNotFound(err) {
return ctrl.Result{}, err
}
util.LogForObject(helper, fmt.Sprintf("Removed finalizer from BarbicanAPI %s", barbicanAPI.Name), barbicanAPI)
}
}

@mauricioharley mauricioharley force-pushed the barbican_keystone_listener branch from 22e3c2c to b0449e3 Compare October 24, 2023 15:08
Comment on lines 52 to 53
// API endpoint
//APIEndpoints map[string]string `json:"apiEndpoint,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove these lines - no api endpoint

type BarbicanKeystoneListenerStatus struct {
// INSERT ADDITIONAL STATUS FIELD - define observed state of cluster
// Important: Run "make" to regenerate code after modifying this file
// ReadyCount of barbican API instances
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

barbican keystone listener

Comment on lines 356 to 372
// TODO(afaranha): Rename Keystone with Barbican
// We need to allow all KeystoneEndpoint and KeystoneService processing to finish
// in the case of a delete before we remove the finalizers. For instance, in the
// case of the Memcached dependency, if Memcached is deleted before all Keystone
// cleanup has finished, then the Keystone logic will likely hit a 500 error and
// thus its deletion will hang indefinitely.
for _, finalizer := range instance.Finalizers {
// If this finalizer is not our KeystoneAPI finalizer, then it is either
// a KeystoneService or KeystoneEndpointer finalizer, which indicates that
// there is more Keystone processing that needs to finish before we can
// allow our DB and Memcached dependencies to be potentially deleted
// themselves
if finalizer != helper.GetFinalizer() {
return ctrl.Result{}, nil
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain to me what this code is supposed to be doing?

envVars := map[string]env.Setter{}
envVars["KOLLA_CONFIG_STRATEGY"] = env.SetValue("COPY_ALWAYS")
envVars["CONFIG_HASH"] = env.SetValue(configHash)
/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets get rid of the commented out parts here. we'll have to figure out probes for these kinds of objects (listener, worker) later, but they won't be these kinds of probes

args := []string{"-c"}
if instance.Spec.Debug.Service {
args = append(args, common.DebugCommand)
//livenessProbe.Exec = &corev1.ExecAction{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

//readinessProbe.Exec = livenessProbe.Exec
} else {
args = append(args, ServiceCommand)
//
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Env: env.MergeEnvs([]corev1.EnvVar{}, envVars),
VolumeMounts: barbican.GetLogVolumeMount(),
Resources: instance.Spec.Resources,
//ReadinessProbe: readinessProbe,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

@@ -21,13 +21,13 @@ interface = internal
{{ end }}

[keystone_notifications]
enable = false
enable = true
topic = notifications
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought we decided to use topic = barbican_notifications so that barbican would get its own listening queue

#driver=messagingv2
driver=noop
driver=messagingv2
#driver=noop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete line

Copy link
Contributor

@vakwetu vakwetu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits and a change for the topic to barbican_notifications

@mauricioharley mauricioharley force-pushed the barbican_keystone_listener branch from 73d7383 to 44ff70b Compare October 26, 2023 10:22
@mauricioharley mauricioharley force-pushed the barbican_keystone_listener branch from 44ff70b to ed1aa45 Compare October 31, 2023 14:37
@vakwetu vakwetu merged commit ba23837 into openstack-k8s-operators:main Oct 31, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants